The per-message size sanity checking commit is also pull request #6261

The mocktime fixes are pull request #6337 : MERGED, commits removed.

The remove coinbase payment key from keypool commit will be obsolete if #5994 is merged : MERGED, commit removed.

Higher-level discussions on the merits of BIP 101 versus other proposals belongs on the bitcoin-dev mailing list

Note to readers: in its current form there is a near zero chance of this getting merged due to a number of BIP-level issues in addition to debate about the patch itself. For instance, Gavin has never given any details about testing; at minimum we'd need a BIP16 style quality assurance document. We also frown on writing software with building expiration dates, let alone expiration dates that trigger non-deterministically. (Note how my recently merged CLTV considered the year 2038 problem to avoid needing a hard fork at that date)

Until these issues are addressed I an many other contributors will be muting this thread and ignoring comments until the BIP itself is fixed. Much of the discussion we see in conversations around this subject is highly repetitive and a big timesink; don't interpret silence as agreement.

There is rough (but not 100%) consensus that at some point fees will rise "high enough" that the 1MB blocksize limit will have to be raised, but no consensus on what that point should be and no consensus on any process to determine what that point should be.

See the mailinglist for why I totally disagree with the fact that fear of higher fees are a reason to increase the block size. If fees rise high enough, and people pay them, that sounds like a fantastic problem to have...

I'm happy to see the code for review openly here, but definitely do not take the block size discussion here.

Only had a minute to look over it, but I added a few comments about code for the sake of reviewing code. Policies and numbers can be discussed elsewhere, it's reasonable to critique the code independent of those discussions.

I'll give it a thorough review over the weekend.

I'm going to delete a few comments from @btcdrak and @rebroad that don't follow the "comment on the code-- high-level discussion goes to bitcoin-dev mailing list"

@theuni : thanks for the review-- this was forward-ported from 0.10 to 0.11 to git HEAD. It would be helpful if there was a design document for libbitcoinconsensus that I could code against...

@gavinandresen Point taken. I'm not sure if we're quite to that point yet, though. We're still in the "move things around so we can do something useful" part and not quite to the "what useful things to do and how" part.

In particular for this PR, consensus functions need to work statelessly, so something like CheckTransaction() should have deterministic results depending only on its parameters and chainparam constants. That is a work-in-progress atm, see #6051 as an example of the current momentum. FWIW, that's why we've started passing chainparams and consensusparams around everywhere.

For the scope of this PR i don't think it's necessary to worry about that too much, but it'd be nice if it didn't make chainparams itself stateful.

That said, these new vars/functions probably belong in consensus/params.h rather than chainparams anyway; that's the new home for consensus-critical stuff.

I'd prefer to see something like:

diff --git a/src/consensus/params.h b/src/consensus/params.h
index c480a1c..b4b97f6 100644
--- a/src/consensus/params.h
+++ b/src/consensus/params.h
@@ -24,7 +24,22 @@ struct Params {
     bool fPowAllowMinDifficultyBlocks;
     int64_t nPowTargetSpacing;
     int64_t nPowTargetTimespan;
+
+    /** Maximum block size parameters */
+    uint32_t nMaxSizePreFork;
+    uint64_t nEarliestSizeForkTime;
+    uint32_t nSizeDoubleEpoch;
+    uint64_t nMaxSizeBase;
+    uint8_t nMaxSizeDoublings;
+    int nActivateSizeForkMajority;
+    uint64_t nSizeForkGracePeriod;
+
     int64_t DifficultyAdjustmentInterval() const { return nPowTargetTimespan / nPowTargetSpacing; }
+    uint64_t MaxTransactionSize(uint64_t nBlockTimestamp, uint64_t nForkTime) const { ... };
+    uint64_t MaxBlockSize(uint64_t nBlockTimestamp, uint64_t nForkTime) const { ... };
+    uint64_t MaxBlockSigops(uint64_t nBlockTimestamp, uint64_t nForkTime) const { return MaxBlockSize(nBlockTimestamp, nForkTime)/50; }
+
+    }
 };
 } // namespace Consensus

concept ACK

Reworked to fix @theuni code review comments:

• Made chainparams constant: main.cpp holds a sizeForkTime global (that behaves like a std::atomic)
• Moved block size fork params/methods to consensus/params.h

Regarding your comment on how peers that send over-maximum-blocks are handled:

That is a behavior change, but only if you send blocks between 1 and 2 megabytes before the fork switchover. Before: Send a block message > 2 MB: you get immediately disconnected (but not banned). Between 1 and 2MB: CheckBlock() would call Misbehaving() and you might get disconnected/banned (if not whitelisted).

With this pull request: any oversize block will get you immediately disconnected.

I started to make the behavior more consistent... but it's complicated because Misbehaving() requires the cs_main lock be held, but the SanityCheckMessage() callback is deep inside the network-servicing code with cs_vRecvMsg lock held. Untangling that is, I think, a project for another day.

... ah, except the java comparison tool doesn't like being disconnected when it sends a big block. That's why Travis is failing.

@theuni : new Travis error:
{standard input}: Fatal error: can't close test/test_test_bitcoin-sanity_tests.o: No space left on device

Is there anything I can do to fix that?

@gavinandresen given that it only happened on the one builder and i've never seen it before, I'd chalk that up to a freak occurrence unless it becomes a trend. Fire it off again?

It was, indeed, a spurious Travis error.

A note on the Java comparison tool: I believe it will continue to work with this pull request "forever" because it produces down-version blocks for testing; it will never trigger the supermajority-of-miners condition to make bigger blocks legal.

Been running on mainnet for 1 day. Got a lot of "Oversized addr" disconnects (65% of all connections, in bursts). Don't know what was being received though. Also got one "Oversized headers" disconnect.

Closing for now; too painful to try to keep up with the movement of code from main.h to policy/policy.h.

Do you guys think this has affected the Bitcoin Protocol?